Implement preopened_dir using set_mapped_directory setting #498
Conversation
|
FYI: the failures on Windows are (unfortunately) expected at this time, I'm working on a fix for those. |
|
The test failures appear to be due to an attempt to borrow a It seems like when pub fn set_mapped_directory(
rb_self: RbSelf,
host_path: RString,
guest_path: RString,
dir_perms: Symbol,
file_perms: Symbol,
) -> RbSelf {
let mut inner = rb_self.inner.borrow_mut(); // We first borrow here
if inner.mapped_directories.is_none() {
inner.mapped_directories = Some(RArray::new().into());
}
// ...
}
// RArray::new() triggers a call to here, where we attempt to borrow it again
impl DataTypeFunctions for WasiConfig {
fn mark(&self, marker: &Marker) {
self.inner.borrow().mark(marker); // panic happens here
}
}I don't have much experience with magnus or the ruby garbage collection (or rust in general), so I am not sure how to resolve this problem. Do you have any siggestions? See the backtrace for more details: Local test run with backtraceroot@b573bf1132ac:/wasmtime-rb# RUST_BACKTRACE=full GC_STRESS=true bundle exec rake spec /usr/bin/ruby3.2 -I/var/lib/gems/3.2.0/gems/rspec-core-3.13.4/lib:/var/lib/gems/3.2.0/gems/rspec-support-3.13.4/lib /var/lib/gems/3.2.0/gems/rspec-core-3.13.4/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb Run options: include {:focus=>true}All examples were filtered out; ignoring {:focus=>true} -- Control frame information ----------------------------------------------- -- Ruby level backtrace information ---------------------------------------- -- C level backtrace information ------------------------------------------- -- Other runtime information -----------------------------------------------
aaaab7760000-aaaab7761000 r-xp 00000000 00:3e 2005802 /usr/bin/ruby3.2 Aborted |
william-stacken
force-pushed
the
preopened-dir
branch
2 times, most recently
from
815d83b to
3fbaf8b
Compare
|
Alright, it appears to solve the problem if the array is allocated before the borrow. |
Add support for directory and file permissions
william-stacken
force-pushed
the
preopened-dir
branch
from
0e0f28a to
cea0c26
Compare
|
I'll take a look at this today! |
ext/src/ruby_api/wasi_config.rs
Outdated
| mapped_directory.push(dir_perms).unwrap(); | ||
| mapped_directory.push(file_perms).unwrap(); | ||
|
|
||
| let init_directory = RArray::new(); |
There was a problem hiding this comment.
Should the creation of this RArray be moved under the if on line 267? Else we'd be creating an array even though it won't be needed.
There was a problem hiding this comment.
Would we consider storing these in a Vec<SomeStruct> instead of nested RArrays?
Pros:
- Sidesteps the refcell+alloc issue altogether (the top-level
RArrayis gone). - Fewer Ruby allocations: no need to alloc an
Arrayto store the arguments for each call. - Errors converting Ruby args into
WasiCtxBuilder.preopened_dir-compatible args will show up immediately, not delayed in a future call. Tiny detail, but makes errors easier to debug.
Cons:
- Requires implementing such struct to hold the arguments, and marking the string it contains.
- Something else I'm not seeing?
I didn't mean to hijack the PR, feel free to ignore if not useful.
There was a problem hiding this comment.
I think the suggestion is reasonable, personally the main advantage that I see, is not having to incur in the overhead of creating a Ruby array each time.
There was a problem hiding this comment.
Good idea, I switched to using a Vec and there are still no garbage collection problems.
william-stacken
force-pushed
the
preopened-dir
branch
from
65bdb3d to
5087b17
Compare
william-stacken
force-pushed
the
preopened-dir
branch
from
5087b17 to
4bd4e38
Compare
ext/src/ruby_api/wasi_config.rs
Outdated
| let host_path = host_path.to_string().unwrap(); | ||
| let guest_path = guest_path.to_string().unwrap(); | ||
| let dir_perms: DirPerms = PermsSymbolEnum(dir_perms).try_into()?; | ||
| let file_perms: FilePerms = PermsSymbolEnum(file_perms).try_into()?; | ||
|
|
||
| let mapped_dir = MappedDirectory { | ||
| host_path, | ||
| guest_path, | ||
| dir_perms, | ||
| file_perms, | ||
| }; | ||
|
|
||
| let mut inner = rb_self.inner.borrow_mut(); | ||
| inner.mapped_directories.push(mapped_dir); |
There was a problem hiding this comment.
To:
- Keep things consistent with the other build options (see e.g.,
env), could we makeMappedDirectoryhold the Ruby values rather than eagerly converting them to Rust on each call? - Related to point 1, that approach will also result in less overhead in case any of the other values fails to validate and centralizing everything in the
buildcall.
|
Great work William, thank you for pushing this over the finish line. |
This is a continuation of #427, I have updated the code and added tests.
Fixes #426